Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Systematize builtins.fetchTree docs #9732

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 9, 2024

Motivation

The renaming task is splatting everything together into markdown lists. I use cmark to then assemble everyting together into Markdown.

Context

I tried using lowdown's AST first, but there were just too many issues, most significantly that lowdown doesn't actually support rendering back to markdown, which is needed for the manual (mdbook renders in that case).

PR #9273 should be able to improve upon this a good bit, but I think it is still a useful stepping stone.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Jan 9, 2024
src/libfetchers/git.cc Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Jan 10, 2024

This seems more productive than what I've been up to with a self-documenting codec abstraction, which may be too ambitious for something in C++.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Jan 12, 2024
@Ericson2314 Ericson2314 force-pushed the systematize-fetchTree-docs branch 4 times, most recently from cc8c940 to a330689 Compare January 17, 2024 00:37
@Ericson2314 Ericson2314 marked this pull request as ready for review January 17, 2024 00:48
@Ericson2314
Copy link
Member Author

OK @roberth @fricklerhandwerk @infinisil the infra here is now set up! Not sure whether we want to

  • Just merge it
  • Hide not-yet-documented fields
  • Go through and document everything

@Ericson2314 Ericson2314 changed the title WIP systematize builtins.fetchTree docs Systematize builtins.fetchTree docs Jan 17, 2024
@Ericson2314 Ericson2314 force-pushed the systematize-fetchTree-docs branch 2 times, most recently from 80aae23 to d4e9043 Compare January 17, 2024 18:15
@Ericson2314 Ericson2314 changed the base branch from master to backport-9461-to-2.19-maintenance January 17, 2024 21:51
@Ericson2314 Ericson2314 changed the base branch from backport-9461-to-2.19-maintenance to master January 17, 2024 21:51
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Jan 18, 2024

I vote to merge as is, and agree it's a useful step forward.

But I sigh loudly as well because it's adding notable tech debt. Now we have 5 (!) mechanisms for producing documentation:

  1. plain markdown via mdBook
  2. globals.hh, the Config object, and --dump-cli
  3. pre-processing in Makefiles
  4. processing in Nix language
  5. this

Especially the additional document composition code path makes it substantially more messy.

The ideal end state for me would be documentation living exclusively in C++ headers (with plain markdown files for optional free-form blurbs) and processing structured output – templating and such – happening exclusively in the Nix language, decoupled from building the all of Nix. Currently it's hacks on top of hacks and it takes forever to compile.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 18, 2024

I agree there are too many. I would like to replace the stuff we do with the interpreter inside Nix with this method, because then it will work with the store-only Nix.

@fricklerhandwerk
Copy link
Contributor

replace the stuff we do with the interpreter inside Nix with this method

That would make it a lot harder to contribute to for most people, and I don't see why docs can't be built by a full Nix as a separate step, but let's discuss it somewhere else.

@thufschmitt
Copy link
Member

thufschmitt commented Jan 19, 2024

Discussed during the maintainers meeting today. Deferred for the time being

  • Adds an extra dep (cmark)
    • Having another markdown renderer in the closure is meh
    • @Ericson2314: Required because manipulating the lowdown AST doesn't work
    • @thufschmitt Embedding the Markdown directly probably works and is 10% of the work, although it's a bit less nice in theory
      • We're already doing it in a bunch of places
  • @Ericson2314: Would be happy to switch to cmark everywhere, but doesn't support outputing to the terminal

Decision: Defer until commonmark/cmark#362 (comment) gets an answer. The the plan is to

  • If the answer is positive, @Ericson2314 will implement the cmark terminal renderer
  • Otherwise just concatenate markdown snippets

@tribals
Copy link

tribals commented Jan 20, 2024

Our use case includes having a self-contained nix executable directly output some pretty-pretty documentation on the terminal without relying on other programs.

Personally I want to state that such style of documentation for CLI tools is worst-case - it is neither man nor help, you're forcing user to pipe output to pager and leaving out man convetions (no one interested in following them because "help files" is written in Markdown, and typical markdown user is rarely even avare of existence of manual pages, and their conventions).

Not to mention that you're forcing user to execute your command in order to get help (rather than "viewing static files" with man).

(From here: commonmark/cmark#362).

@roberth
Copy link
Member

roberth commented Jan 20, 2024

Lowdown represents one local optimum, where it's responsible for the whole pipeline. This is insufficient, as described by tribals just now.

Another optimum is one where we have

  • A markdown parser
  • A semantically rich document AST, suitable for man page generation
  • A good markdown writer, from markdown AST => cmark
  • A function from document AST to markdown AST
  • A terminal layout library => a Wadler-style pretty printer
    • for printing values and expressions
    • for :doc

In this scenario, we've got rid of

  • Ad hoc stringly markdown manipulation that requires a bunch of rendering and visual review
  • The need for a direct markdown to terminal renderer. We reuse our existing rendering framework for expressions, and the translation from the limited number of markdown constructors to the layout+terminal framework should be fairly simple.

Do we want capable documentation tooling? If so, what are the steps to bridge the optima? Adding cmark could be step one, but we should only take it if we agree on such a vision.

@roberth
Copy link
Member

roberth commented Jan 20, 2024

For yet another optimum, we could consider minimizing the documentation related code in Nix, and generate the documentation as well as some code from a single source that isn't C++.

I do think we eventually want nice expression printing and such, so it feels like #9817 might be a detour.

@tribals
Copy link

tribals commented Jan 20, 2024

Personally I, again, would recommend to move from Markdown (it is not Holly Graal) to something more suitable for writing documentation, eg. https://gitlab.com/ddevault/scdoc/-/blob/master/scdoc.5.scd

Please note that I'm random outsider, brought to this issue by accident, so don't take my musings for granted.

I have rare experience with Nix, and want to state that CLI tooling was one reason why I switched to Guix.

@roberth
Copy link
Member

roberth commented Jan 20, 2024

The Nix ecosystem has by and large switch to markdown in order to make writing easier, reasoning that lowering the contribution threshold leads to more end user improvements than fancy features such as table captions. Indeed we have recognized that it is not the Holy Grail, and we've decided that retrieving that artifact is not feasible, and focus on things that matter more.

However, it is a fact that markdown is of little use when writing highly structured documentation, such as manpages. It needs to be embedded in something richer, and/or it needs to be extended with custom syntax, like we do in Nixpkgs Markdown, with e.g. {option}`services.postgresql.enable` .

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-25-documentation-team-meeting-notes-106/38792/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-19-nix-team-meeting-minutes-116/38837/1

@Ericson2314
Copy link
Member Author

I can remove the patch now that commonmark/cmark#524 is merged --- thanks CMark maintainers! But I still need to do something about replacing lowdown.

@Ericson2314
Copy link
Member Author

As I suspected, cmark wasn't interested in accepting 2000 more lines of C from lowdown, which means there is not an immediate path to getting rid of the lowdown dependency.

I would still like to merge this as is, however:

  • AST manipulation in C++ is a good thing to be doing, and indeed will help with other efforts like the store-only Nix CLI.

  • Having two Markdown libraries might feel icky, but it doesn't actually pose any practical problem.

  • @roberth's idea of replacing lowdown with a C++ pretty printing library using the cmark AST (which can include custom nodes, which we've leveraged in e.g. the Nixpkgs documentation) would make an excellent self-contained GSoC project

`cmark` is a more robust library for constructing Markdown than
`lowdown`. It is also more portable. Unfortunately it doesn't have a
terminal mode, so we cannot get rid of lowdown yet.

The `--enable-markdown` flag is renamed to `--enable-lowdown`
accordingly.
Needed until commonmark/cmark#524 is released.

• Added input 'cmark':
    'github:commonmark/cmark/cd37711b8a08da67ba4e21a42614b86dd8def929' (2024-01-26)
The renaming task is splatting everything together into markdown lists.
The docs are temporily not rendered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants